fix: remove mention of disabling file locking from config sample #45330
Conversation
icewind1991
requested review from
a team,
ArtificialOwl,
yemkareems and
sorbaugh
and removed request for
a team
| @@ -2185,21 +2185,6 @@ | |||
| */ | |||
| 'max_filesize_animated_gifs_public_sharing' => 10, | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
GitHub won't let me make suggestions on deleted lines, but what if instead of removing it we adjusted the language to be a bit more scary like the documentation already is + tried to emphasize it's not the same as files_lock:
/**
* Transactional file locking
* Locks files to avoid file corruption during normal operation.
*
* WARNING: Transactional file locking is not for preventing multiple users from editing
* the same document, or giving notice that other users are working on the same document.
* Multiple users can open and edit a file at the same time and Transactional File
* locking does not prevent this. Rather, it prevents simultaneous file saving. This
* is enabled by default and should generally be left enabled.
*
* IMPORTANT: If you are experiencing problems with user facing locking, such
* as to avoid simultaneously edits, you may be looking for the `files_lock` app and
* its settings and/or settings within the client apps you're using. Do not disable this
* parameter unless you are extremely sure you know what you're doing.
There was a problem hiding this comment.
I would rather also remove the mention of the option from the documentation. I can't think of a single case where disabling it would be advisable.
There was a problem hiding this comment.
Makes sense. 👍
The toggle was only added for turning it on from the looks of it (when it wasn't on by default since it was still experimental at the time): 2f4f468
That need no longer applies today.
Made sense to probably keep it around for a bit when it switched to being on by default in 4880d77, but those days are long over. :-)
| @@ -2185,21 +2185,6 @@ | |||
| */ | |||
| 'max_filesize_animated_gifs_public_sharing' => 10, | |||
|
|
|||
|
|
|||
There was a problem hiding this comment.
Makes sense. 👍
The toggle was only added for turning it on from the looks of it (when it wasn't on by default since it was still experimental at the time): 2f4f468
That need no longer applies today.
Made sense to probably keep it around for a bit when it switched to being on by default in 4880d77, but those days are long over. :-)
|
We can probably pull the unit test override too since it's extraneous: Line 1088 in 5a6e48e |
No longer relevant today since it's not an experiment feature and is now on by default. There is no good reason to disable it. See #45330 & nextcloud/server#45330 (comment) Signed-off-by: Josh <[email protected]>
|
I took care of the Transactional File Locking docs chapter: nextcloud/documentation#11848 |
No longer relevant today since it's not an experiment feature and is now on by default. There is no good reason to disable it. See #45330 & nextcloud/server#45330 (comment) Signed-off-by: Josh <[email protected]>
No longer relevant today since it's not an experiment feature and is now on by default. There is no good reason to disable it. See #45330 & nextcloud/server#45330 (comment) Signed-off-by: Josh <[email protected]>
skjnldsv
added
2. developing
Signed-off-by: Robin Appelman <[email protected]>
icewind1991
force-pushed
the
remove-locking-config-sample
branch
from
8e6ba4f
to
6791ecb
Compare
Disabling transactional locking is almost always a bad idea and can easily lead to data corruption or another range of unexpected behavior.
Removing the mention of this footgun hopefully reduces the number of people disabling the locking without proper understanding.